Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move IsoDatastore from core #22239

Merged
merged 2 commits into from
Dec 14, 2022
Merged

Move IsoDatastore from core #22239

merged 2 commits into from
Dec 14, 2022

Conversation

nasark
Copy link
Member

@nasark nasark commented Nov 17, 2022 •
edited
Loading

  • since iso datastores are only used by oVirt/RHV, we can move its model from core and into the provider plugin
  • added associations to model iso_images -> storage relationship

Depends on:

@miq-bot add_labels pluggability/providers, technical debt, enhancement
@miq-bot assign @agrare

@nasark nasark mentioned this pull request Nov 17, 2022
Merged
9 tasks
@kbrock
Copy link
Member

kbrock commented Nov 17, 2022 •
edited
Loading

Lets keep in mind that our view layer has trouble looking up these relationships if it is not defined in the base class that is used for the query.

meaning that after this change:

ExtManagementSystem.includes(:iso_datastore).all # fails
RedHat::Ovirt::ExtManagementSystem.includes(:iso_datastore).all # succeeds

Any views that are based upon EMS will no longer be able to display this data, even if only OVirt providers are displayed.

But that may not be a real use case and we may only be viewing these fields from an OVirt provider.

UPDATE: I am excited for this change.
I think we will be using includes(:datastores) instead, so my point is moot.

@nasark nasark force-pushed the move_iso_datastore branch from 8ee7e78 to df2b301 Compare November 23, 2022 20:53
@nasark nasark force-pushed the move_iso_datastore branch from df2b301 to 4721129 Compare December 5, 2022 15:38
agrare
agrare reviewed Dec 5, 2022
View reviewed changes
app/models/iso_image.rb
@@ -1,5 +1,5 @@
class IsoImage < ApplicationRecord
belongs_to :iso_datastore
belongs_to :storage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this because it opens us up to creating these from other provider storage types like VMware when we do a datastore ssa. This would make it a lot simpler to get a list of ISOs when doing vm reconfigure to connect a cdrom or provision from an ISO

@dberg1 dberg1 mentioned this pull request Dec 6, 2022
Merged
1 task
@agrare
Copy link
Member

agrare commented Dec 6, 2022

@nasark can you double check that there are no more references to IsoDatastore?
E.g. I see

manageiq/app/models/ext_management_system.rb

Lines 995 to 998 in d178e29

# @return [Boolean] true if a datastore exists for this type of ems
def self.datastore?
IsoDatastore.where(:ems_id => all).exists?
end
isn't deleted by this PR

@nasark nasark force-pushed the move_iso_datastore branch from d178e29 to 7117e9a Compare December 7, 2022 15:31
@nasark nasark requested a review from jrafanie as a code owner December 7, 2022 15:31
@kbrock
Copy link
Member

kbrock commented Dec 7, 2022 •
edited
Loading

may also want to grep for iso_datastore

found

app/models/miq_request_workflow.rb
1408:    template.try(:ext_management_system).try(:iso_datastore).try(:iso_images) || []

and noticed (but not sure the ramifications)

  supports_not :create_iso_datastore

@agrare
Copy link
Member

agrare commented Dec 9, 2022

Good catch @kbrock I think we'll want template.try(:ext_management_system).try(:iso_images) || [] with a has_many :iso_images, :through => :storages

@nasark nasark force-pushed the move_iso_datastore branch from 7117e9a to 98977e0 Compare December 12, 2022 20:40
@miq-bot
Copy link
Member

miq-bot commented Dec 12, 2022

Checked commits nasark/manageiq@d7081e7~...98977e0 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
5 files checked, 0 offenses detected
Everything looks fine. 🍪

@agrare agrare closed this Dec 14, 2022
@agrare agrare reopened this Dec 14, 2022
@agrare agrare merged commit 4d7e1f6 into ManageIQ:master Dec 14, 2022
@Fryguy Fryguy added this to the Petrosian milestone Sep 14, 2023
@agrare agrare mentioned this pull request Mar 14, 2024
Open
48 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agrare agrare agrare approved these changes

@Fryguy Fryguy Awaiting requested review from Fryguy Fryguy is a code owner

@kbrock kbrock Awaiting requested review from kbrock kbrock is a code owner

@jrafanie jrafanie Awaiting requested review from jrafanie jrafanie is a code owner

Assignees

@agrare agrare

Labels
enhancement pluggability/providers technical debt
Projects
Roadmap
Status: Petrosian
Milestone
Petrosian
Development

Successfully merging this pull request may close these issues.
5 participants
@nasark @kbrock @agrare @miq-bot @Fryguy